Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update jupyter_ydoc and pycrdt_websocket dependencies #367

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Oct 10, 2024

This PR updates dependencies:

  • jupyter_ydoc >=2.1.2
  • pycrdt_websocket >=0.15.0
  • indirectly pycrdt >=0.10.1,<0.11.0

These changes allow the use of document awareness on the server side, to handle states on server.

Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/jupyter_collaboration/ydoc_awareness

@brichet brichet added enhancement New feature or request maintenance and removed maintenance labels Oct 10, 2024
@brichet brichet marked this pull request as ready for review October 10, 2024 14:48
if type != "change":
return
added_users = changes[0]["added"]
removed_users = changes[0]["removed"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "updated" users?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I wonder if the connected_users of the _websocket_server is even used.
I copied that code from the previous message handler, but I don't know if we need that function.
If we keep it, we should indeed handle the "updated" users (removing the former name and add the new one).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep it, we should indeed handle the "updated" users (removing the former name and add the new one).

Thinking again about that, we don't have the previous name, the changes contain only the client ids.
We should rebuild the full list from the global awareness when a user is updated, because we don't know if the user name has been updated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the goal of this code was to check in the backend that the awareness information from the frontend is correct. For instance, we don't want a student to take the user name of the teacher. That's why there is this skip variable that was supposed to filter out an awareness message, but this was never put to actual use.
I'm wondering how we can filter out a message with your changes though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we can filter out a message with the current information we have.
For example when you reload the page, it adds a new client id with the same user information. The old client id is removed in a second step (probably when a client has a lack of update from it ?).
The same user is duplicated over a period of time, with 2 different client IDs. If we allow this behavior, I don't know how we can filter out someone trying to cheat.

The following image shows some logs when a remote client reloads the page. When a change is received, the change and the current users in the awareness are printed.
Screenshot from 2024-10-15 16-10-45

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend is the authority for user identities, and the frontend can get its identity at /api/me, so we should be able to check that they match.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's out of scope for this PR, but I'm wondering if we still get the possibility to discard an awareness update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants